Skip to content

fix(codegen): root_reload's cost cap counts root loads, not derived values - #9854

Closed
proggeramlug wants to merge 1 commit into
PerryTS:mainfrom
proggeramlug:fix/root-reload-cap-counts-root-loads
Closed

fix(codegen): root_reload's cost cap counts root loads, not derived values#9854
proggeramlug wants to merge 1 commit into
PerryTS:mainfrom
proggeramlug:fix/root-reload-cap-counts-root-loads

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

The bug

MAX_BLOCK_LOAD_PRODUCT guards the reachability walk in root_reload::apply_to_function. That walk runs once per groups entry — one per root load; the module's own comment says so ("Grouping by root load also puts the cost back at O(blocks × loads)").

The check multiplied by values.len() instead. Since #7664 extended a recipe through pure bit ops, values counts every derivation as its own Reloadable, so on a wide function it is several times the group count. The pass therefore declined on functions whose actual cost was well inside the bound.

Why declining is not safe

The constant's comment read:

Above the cap the function keeps today's IR — the pass is an improvement, not a correctness precondition, so declining is safe.

Under the native root lowering that is false, and it is why this went unnoticed. When the pass does not run, nothing else re-reads the slot. A receiver read out of its root and unmasked to an i64/double is exactly the "case 2" shape this module's header documents: RS4GC relocates the addrspace(1) load, but the unmasked copy that actually crosses the call is beyond its reach.

Where it was found

Claude-of-Duty's Arm.constructor:

metric value vs 8M cap
blocks × values (old) 4924 × 2102 = 10,350,248 over → declined
blocks × groups (new) 4924 × 747 = 3,678,228 under

With the pass declined, this.upper = buildSleeve(...) stored through a stale receiver. Under PERRY_GC_PROTECT_FROMSPACE=1:

[gc-fromspace-protect] FAULT: signal 10
  This address is RETIRED FROM-SPACE. The evacuating minor moved or
  freed the object here and the holder kept the pre-collection address.
  last-known object: obj_type=2 size=184
  perry_..._Arm_constructor  <-  Viewmodel_constructor  <-  WeaponSystem__init

The faulting instruction is the inline class-field store's own shape check, ldurh w8, [x19, #-0x6], where x19 is the receiver unmasked from callee-saved x22 — loaded before bl buildSleeve and never relocated. Without from-space protection it degrades silently: the write lands in the dead copy, so the later read of this.upper returns undefined and THREE reports Object3D.add: object not an instance of THREE.Object3D. undefined.

The change

One term. groups is built before the cap check and the product measured against it. The bound is unchanged at 8M.

A function that genuinely exceeds blocks × groups still declines and can still carry a stale register. That residual risk is real; the constant's doc now states it instead of denying it.

Test

the_cap_counts_root_loads_not_derived_values replicates the existing masked_receiver shape across 1100 blocks, each with a MAX_RECIPE-length derivation, sized to clear the cap by the old metric (~9.7M) and sit an order of magnitude inside it by the new one (~1.2M).

Verified in both directions:

rewrites
values.len() (before) 0 — the pass declines the whole function
groups.len() (after) 1100

A note on the checker

scripts/gc_root_dominance_check.py --stale-registers --moving-only does not flag this shape. On the faulting module it reported 0 stale uses of this kind (17 total, all source=global). So it cannot be relied on as a guard here — worth a separate look.

Verification

  • cargo test --release -p perry-codegen — 1912 passed, 0 failed
  • cargo test --release -p perry-runtime — 2878 passed, 0 failed (on the earlier base)
  • End to end: the reduced reproducer goes 138 → 0 with no from-space fault, and Claude-of-Duty's native build now gets past WeaponSystem.init (3 weapons · 136.8k tris viewmodel) and renders.

Summary by CodeRabbit

  • Bug Fixes
    • Improved code generation for functions with many control-flow blocks and derived values.
    • Prevented valid functions from being rejected by an overly restrictive reload-cost limit.
    • Ensured stale values remain correctly reloaded across operations that may trigger garbage collection.
    • Added regression coverage for large functions involving masked value derivations and collecting calls.

…alues

`MAX_BLOCK_LOAD_PRODUCT` guards the reachability walk in `apply_to_function`,
and that walk runs once per `groups` entry — one per ROOT LOAD. The check
multiplied by `values.len()` instead, which since PerryTS#7664 counts every pure-bit-op
derivation as its own `Reloadable`. On a wide function that is several times the
group count, so the pass declined on functions whose real cost was well inside
the bound.

Declining is not correctness-neutral under the native root lowering. The
constant's comment claimed "the pass is an improvement, not a correctness
precondition, so declining is safe"; that is false, and it is why the cliff went
unnoticed. When the pass does not run, nothing re-reads the slot: a receiver
read out of its root, unmasked to an i64/double and carried across a call is a
value RS4GC cannot relocate, so the store lands in a from-space object.

Found on Claude-of-Duty's `Arm.constructor` (4924 blocks, 747 root loads, 2102
values): 10,350,248 by the old metric against an 8M cap, 3,678,228 by the new
one. It declined, and `this.upper = buildSleeve(...)` wrote through a stale
receiver — a SIGBUS under `PERRY_GC_PROTECT_FROMSPACE=1`, and silent field
corruption without it (`THREE.Object3D.add: object not an instance of
THREE.Object3D. undefined` two frames later).

The bound itself is unchanged; only the term it is measured against. A function
that genuinely exceeds `blocks x groups` still declines and can still carry a
stale register — that residual risk is now stated at the constant rather than
denied.

Note that `scripts/gc_root_dominance_check.py --stale-registers --moving-only`
does NOT flag this shape: it reported 0 stale uses on the faulting module (17
found, all `source=global`), so it cannot serve as a guard here.

The regression test replicates the `masked_receiver` shape across 1100 blocks
with a MAX_RECIPE-length derivation, sized to clear the cap by the old metric
and sit an order of magnitude inside it by the new one. It inserts 0 reloads
before this change and 1100 after.
@coderabbitai

coderabbitai Bot commented Sep 6, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Team

Run ID: 3a26506d-954b-47c5-99d3-4d33036b596b

📥 Commits

Reviewing files that changed from the base of the PR and between bcce8de and 15f4b19.

📒 Files selected for processing (2)
  • crates/perry-codegen/src/root_reload.rs
  • crates/perry-codegen/src/root_reload_tests.rs

Included review availability: Your plan provides up to 8 included reviews per hour; 4 remain after this review.


📝 Walkthrough

Walkthrough

The root reload pass now measures its reachability cap using distinct root-load groups instead of all reloadable values. A regression test covers masked derivations across 1,100 blocks and verifies that stale operands are reloaded.

Changes

Root Reload Reachability Cap

Layer / File(s) Summary
Measure the cap by reachability groups
crates/perry-codegen/src/root_reload.rs
The cap now uses blocks × groups, where groups matches the reachability walk driver. The documentation describes the correctness impact of declining the pass.
Validate derived-value handling
crates/perry-codegen/src/root_reload_tests.rs
A regression test verifies reload behavior for masked derivations across 1,100 blocks. It distinguishes root-load groups from derived values when evaluating the cap.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 15f4b

This change prevents root reload from skipping functions solely because derived values inflate the cost estimate, with regression coverage confirming stale operands are reloaded across collecting calls. The change is ready to merge.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: the root reload cost cap now counts root-load groups instead of derived values.
Description check ✅ Passed The description is detailed, relevant, and covers the bug, impact, implementation, regression test, and verification results. It does not use the template headings and omits an explicit related issue …
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@proggeramlug

Copy link
Copy Markdown
Contributor Author

Landed on main via merge train #9875. Validated as a tree: 64/64 lint gates, and perry-runtime/codegen/hir/stdlib all green (5,891 tests, 0 failures). Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant